Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Test extended to verify audit logs. #2153

Merged
merged 6 commits into from
Nov 4, 2022
Merged

Test extended to verify audit logs. #2153

merged 6 commits into from
Nov 4, 2022

Conversation

lukasz-soszynski-eliatra
Copy link
Contributor

Description

[Describe what this change achieves]

  • Category (Integration tests)
  • Why these changes are required?
  • What is the old behavior before changes and new behavior after changes?

Integration tests and test framework extended so that verification of audit logs can be performed in course of the tests.

Issues Resolved

[List any issues this PR will resolve]

Is this a backport? If so, please add backport PR # and/or commits #

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@lukasz-soszynski-eliatra lukasz-soszynski-eliatra requested a review from a team October 11, 2022 14:01
@lukasz-soszynski-eliatra lukasz-soszynski-eliatra mentioned this pull request Oct 11, 2022
Merged
3 tasks
peternied
peternied reviewed Oct 12, 2022
View reviewed changes
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How has the test runtime been impacted by these changes? It looks like there is heavy cpu usage, and possible overhead from pooling.

src/integrationTest/java/org/opensearch/test/framework/TestSecurityConfig.java Outdated Show resolved Hide resolved
src/integrationTest/java/org/opensearch/test/framework/TestSecurityConfig.java Outdated Show resolved Hide resolved
src/integrationTest/java/org/opensearch/test/framework/audit/AuditLogsRule.java Outdated Show resolved Hide resolved
src/integrationTest/java/org/opensearch/security/SearchOperationTest.java Show resolved Hide resolved
@lukasz-soszynski-eliatra lukasz-soszynski-eliatra force-pushed the audit-logs branch 2 times, most recently from c6d4255 to 4a48848 Compare October 13, 2022 15:28
peternied
peternied previously approved these changes Oct 17, 2022
View reviewed changes
@peternied
Copy link
Member

@lukasz-soszynski-eliatra Could you please rebase against main, the build breaks have been fixed

@lukasz-soszynski-eliatra
Copy link
Contributor Author

code rebased into the newest main branch

@peternied
Copy link
Member

Looks like there is a CI failure, can you dig in on that?

@lukasz-soszynski-eliatra
Copy link
Contributor Author

I checked the log output. During the build, many tests failed. The log output contains many error messages related to missing resources like

com.amazon.dlic.auth.ldap.LdapBackendIntegTest > testIntegLdapAuthenticationSSLFail STANDARD_OUT
Error: 22-10-19T12:30:05,391][ERROR][org.opensearch.security.test.helper.file.FileHelper] Failed to load ldap/nodes_dn.yml
Error: 22-10-19T12:30:05,394][ERROR][org.opensearch.security.test.helper.file.FileHelper] Failed to load ldap/whitelist.yml
Error: 22-10-19T12:30:05,394][ERROR][org.opensearch.security.test.helper.file.FileHelper] Failed to load ldap/allowlist.yml
Error: 22-10-19T12:30:05,395][ERROR][org.opensearch.security.test.helper.file.FileHelper] Failed to load ldap/audit.yml

Another messages also point out that cluster configuration is not valid:

Error: 22-10-19T12:29:46,331][ERROR][com.amazon.dlic.auth.http.jwt.HTTPJwtAuthenticator] signingKey must not be null or empty. JWT authentication will not work
Error: 22-10-19T12:29:46,332][ERROR][com.amazon.dlic.auth.http.jwt.HTTPJwtAuthenticator] Missing Signing Key. JWT authentication will not work

Furthermore, many messages which occur multiple times point out that security plugin configuration was not loaded into the index

Error: 22-10-19T12:34:06,354][ERROR][org.opensearch.security.configuration.ConfigurationLoaderSecurity7] Failure No shard available for [org.opensearch.action.get.MultiGetShardRequest@5a020f43] retrieving configuration for [INTERNALUSERS, ACTIONGROUPS, CONFIG, ROLES, ROLESMAPPING, TENANTS, NODESDN, WHITELIST, ALLOWLIST, AUDIT] (index=.opendistro_security)
Error: 22-10-19T12:34:06,354][ERROR][org.opensearch.security.configuration.ConfigurationLoaderSecurity7] Failure No shard available for [org.opensearch.action.get.MultiGetShardRequest@5a020f43] retrieving configuration for [INTERNALUSERS, ACTIONGROUPS, CONFIG, ROLES, ROLESMAPPING, TENANTS, NODESDN, WHITELIST, ALLOWLIST, AUDIT] (index=.opendistro_security)
Error: 22-10-19T12:34:06,354][ERROR][org.opensearch.security.configuration.ConfigurationLoaderSecurity7] Failure No shard available for [org.opensearch.action.get.MultiGetShardRequest@5a020f43] retrieving configuration for [INTERNALUSERS, ACTIONGROUPS, CONFIG, ROLES, ROLESMAPPING, TENANTS, NODESDN, WHITELIST, ALLOWLIST, AUDIT] (index=.opendistro_security)

My supposition is that tests were not able to create configuration files during the build. I am not sure why. I think that these failures are not related to changes introduced in the scope of this PR because older test which failed was not modified.

@lukasz-soszynski-eliatra
Copy link
Contributor Author

@peternied Could you try to rerun the build in CI/CD environment, please?

@peternied
Copy link
Member

I've retried the CI. Network seems to be really flaky over the past 24 hours... on a related note; I've got a pull request that should deal with some of the failing audit tests, if you are interested I could use more eyes on a pull request I have out [1], I also opened an issue to make sure these tests that you have been added also have retries enabled [2]

[1] #2180
[2] #2184

@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2022 •
edited
Loading

Codecov Report

Merging #2153 (b43df6a) into main (a57fd0a) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #2153      +/-   ##
============================================
- Coverage     61.05%   61.02%   -0.03%     
+ Complexity     3270     3267       -3     
============================================
  Files           259      259              
  Lines         18335    18335              
  Branches       3248     3248              
============================================
- Hits          11194    11189       -5     
- Misses         5555     5561       +6     
+ Partials       1586     1585       -1
Impacted Files Coverage Δ
...ecurity/configuration/ConfigurationRepository.java 72.13% <0.00%> (-2.19%) ⬇️
...iance/ComplianceIndexingOperationListenerImpl.java 61.76% <0.00%> (-1.48%) ⬇️
.../org/opensearch/security/support/ConfigHelper.java 86.00% <0.00%> (ø)
...ensearch/security/ssl/DefaultSecurityKeyStore.java 67.83% <0.00%> (ø)
...dlic/auth/http/saml/AuthTokenProcessorHandler.java 47.28% <0.00%> (ø)
...ty/configuration/ConfigurationLoaderSecurity7.java 70.43% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

peternied
peternied previously approved these changes Oct 21, 2022
View reviewed changes
peternied
peternied previously approved these changes Oct 24, 2022
View reviewed changes
DarshitChanpura
DarshitChanpura previously approved these changes Nov 2, 2022
View reviewed changes
build.gradle Show resolved Hide resolved
src/integrationTest/java/org/opensearch/test/framework/TestSecurityConfig.java Outdated Show resolved Hide resolved
DarshitChanpura
DarshitChanpura approved these changes Nov 4, 2022
View reviewed changes
Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. CI failure is due to unrelated issue

@peternied peternied merged commit bd43755 into opensearch-project:main Nov 4, 2022
@DarshitChanpura DarshitChanpura mentioned this pull request Aug 22, 2023
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DarshitChanpura DarshitChanpura DarshitChanpura approved these changes

@peternied peternied peternied approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lukasz-soszynski-eliatra @peternied @codecov-commenter @DarshitChanpura